-
Notifications
You must be signed in to change notification settings - Fork 293
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add csv export for assets #1392
Conversation
WalkthroughThis pull request introduces a new CSV export functionality for assets in the backend and frontend components. A new method, Changes
Sequence DiagramsequenceDiagram
participant User
participant Frontend
participant Backend
participant Database
User->>Frontend: Request CSV Export
Frontend->>Backend: GET /export with model
Backend->>Database: Fetch Accessible Assets
Database-->>Backend: Return Asset Data
Backend->>Backend: Generate CSV
Backend-->>Frontend: CSV Response
Frontend-->>User: Download CSV File
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/src/routes/(app)/(internal)/[model=urlmodel]/export/+server.ts (1)
Line range hint
14-21
: Sanitize the filename to ensure compatibility.The current filename includes special characters from
toISOString()
which might cause issues. Consider sanitizing it.- const fileName = `${URLModel}-${new Date().toISOString()}.csv`; + const timestamp = new Date().toISOString().replace(/[:.]/g, '-'); + const fileName = `${URLModel}-${timestamp}.csv`; return new Response(await res.blob(), { headers: { 'Content-Type': 'text/csv', 'Content-Disposition': `attachment; filename="${fileName}"` } });frontend/src/routes/(app)/(third-party)/[model=thirdparty_urlmodels]/+page.svelte (1)
Line range hint
69-75
: Consider extracting shared export button logic.The export button condition
['applied-controls', 'assets'].includes(URLModel)
is duplicated across components. Consider extracting this into a shared constant or utility function.Also, enhance accessibility by adding an aria-label to the export button:
<a href="{URLModel}/export/" class="inline-block p-3 text-gray-50 bg-pink-500 hover:bg-pink-400 w-12 focus:relative" title={m.exportButton()} + aria-label={m.exportButton()} data-testid="export-button"><i class="fa-solid fa-download mr-2" /></a >
frontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.svelte (1)
Line range hint
150-157
: Align button styling with third-party component.The export button uses different styling classes between internal and third-party components. Consider using consistent styling:
- Internal:
btn-mini-tertiary
- Third-party:
text-gray-50 bg-pink-500 hover:bg-pink-400
Also, add aria-label for accessibility:
<a href="{URLModel}/export/" class="inline-block p-3 btn-mini-tertiary w-12 focus:relative" title={m.exportButton()} + aria-label={m.exportButton()} data-testid="export-button"><i class="fa-solid fa-download mr-2" /></a >
backend/core/views.py (1)
511-550
: Implementation looks good with some suggestions for improvement.The CSV export functionality is well-implemented with proper permission checks and data handling. Consider these improvements:
- For large datasets, consider implementing pagination or streaming to reduce memory usage.
- Add error handling for potential exceptions during CSV generation.
Example implementation with streaming and error handling:
@action(detail=False, name="Export assets as CSV") def export_csv(self, request): try: (viewable_assets_ids, _, _) = RoleAssignment.get_accessible_object_ids( Folder.get_root_folder(), request.user, Asset ) response = HttpResponse(content_type="text/csv") response["Content-Disposition"] = 'attachment; filename="assets_export.csv"' writer = csv.writer(response, delimiter=";") columns = [ "internal_id", "name", "description", "type", "security_objectives", "disaster_recovery_objectives", "link", "owners", "parent_assets", "labels", ] writer.writerow(columns) + # Use iterator() to reduce memory usage + for asset in Asset.objects.filter(id__in=viewable_assets_ids).iterator(): row = [ asset.id, asset.name, asset.description, asset.type, asset.get_security_objectives_display(), asset.get_disaster_recovery_objectives_display(), asset.reference_link, ",".join([o.email for o in asset.owner.all()]), ",".join([o.name for o in asset.parent_assets.all()]), ",".join([o.label for o in asset.filtering_labels.all()]), ] writer.writerow(row) return response + except Exception as e: + logger.error(f"Error exporting assets to CSV: {str(e)}") + return HttpResponse( + status=500, + content="An error occurred while generating the CSV export." + )🧰 Tools
🪛 Ruff (0.8.2)
514-514:
Asset
may be undefined, or defined from star imports(F405)
534-534:
Asset
may be undefined, or defined from star imports(F405)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/core/views.py
(1 hunks)frontend/src/app.postcss
(1 hunks)frontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.svelte
(1 hunks)frontend/src/routes/(app)/(internal)/[model=urlmodel]/export/+server.ts
(1 hunks)frontend/src/routes/(app)/(third-party)/[model=thirdparty_urlmodels]/+page.svelte
(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/core/views.py
514-514: Asset
may be undefined, or defined from star imports
(F405)
534-534: Asset
may be undefined, or defined from star imports
(F405)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
🔇 Additional comments (2)
frontend/src/app.postcss (1)
27-29
: LGTM! The new button style follows existing patterns.The
.btn-mini-tertiary
class maintains consistency with existing button styles while providing good contrast and hover states.frontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.svelte (1)
Line range hint
158-164
: LGTM! Graph visualization button is well-implemented.The graph visualization button for assets is properly implemented with consistent styling and clear purpose.
export const GET: RequestHandler = async ({ params, fetch }) => { | ||
const URLModel = params.model; | ||
const endpoint = `${BASE_API_URL}/${URLModel}/export_csv/`; | ||
|
||
const res = await fetch(endpoint); | ||
if (!res.ok) { | ||
error(400, 'Error fetching the CSV file'); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Enhance error handling and add model validation.
The error handling could be more specific to help debug issues. Also, consider validating the model parameter.
export const GET: RequestHandler = async ({ params, fetch }) => {
const URLModel = params.model;
+ const validModels = ['applied-controls', 'assets'];
+ if (!validModels.includes(URLModel)) {
+ error(400, `Invalid model: ${URLModel}`);
+ }
+
const endpoint = `${BASE_API_URL}/${URLModel}/export_csv/`;
const res = await fetch(endpoint);
if (!res.ok) {
- error(400, 'Error fetching the CSV file');
+ error(res.status, `Failed to fetch CSV: ${await res.text()}`);
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
export const GET: RequestHandler = async ({ params, fetch }) => { | |
const URLModel = params.model; | |
const endpoint = `${BASE_API_URL}/${URLModel}/export_csv/`; | |
const res = await fetch(endpoint); | |
if (!res.ok) { | |
error(400, 'Error fetching the CSV file'); | |
} | |
export const GET: RequestHandler = async ({ params, fetch }) => { | |
const URLModel = params.model; | |
const validModels = ['applied-controls', 'assets']; | |
if (!validModels.includes(URLModel)) { | |
error(400, `Invalid model: ${URLModel}`); | |
} | |
const endpoint = `${BASE_API_URL}/${URLModel}/export_csv/`; | |
const res = await fetch(endpoint); | |
if (!res.ok) { | |
error(res.status, `Failed to fetch CSV: ${await res.text()}`); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/core/views.py (2)
511-564
: Good implementation with room for improvement.The CSV export implementation is solid with proper permission handling, error handling, and memory-efficient iteration. Consider these enhancements:
- Make the filename more descriptive by including a timestamp and domain/folder name.
- Allow configurable CSV delimiter through settings or request parameters.
- Add content encoding specification to handle special characters correctly.
- Consider implementing progress tracking for large datasets.
Here's a suggested improvement:
- response["Content-Disposition"] = 'attachment; filename="assets_export.csv"' + timestamp = timezone.now().strftime("%Y%m%d_%H%M%S") + filename = f"assets_export_{timestamp}.csv" + response["Content-Disposition"] = f'attachment; filename="{filename}"' + response["Content-Type"] = "text/csv; charset=utf-8"🧰 Tools
🪛 Ruff (0.8.2)
515-515:
Asset
may be undefined, or defined from star imports(F405)
535-535:
Asset
may be undefined, or defined from star imports(F405)
Line range hint
1-1
: Replace star imports with explicit imports for better maintainability.The static analysis tool indicates that
Asset
may be undefined. While it's imported through.models import *
, explicit imports make dependencies clearer and improve code maintainability.Replace the star import with explicit imports:
-from .models import * +from .models import ( + Asset, + Evidence, + FilteringLabel, + # Add other required models... +)🧰 Tools
🪛 Ruff (0.8.2)
509-509:
Asset
may be undefined, or defined from star imports(F405)
515-515:
Asset
may be undefined, or defined from star imports(F405)
535-535:
Asset
may be undefined, or defined from star imports(F405)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/views.py
(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/core/views.py
515-515: Asset
may be undefined, or defined from star imports
(F405)
535-535: Asset
may be undefined, or defined from star imports
(F405)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: Analyze (python)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: Analyze (javascript-typescript)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary by CodeRabbit
Release Notes
New Features
UI Improvements
.btn-mini-tertiary
.Backend Enhancements
The release introduces more flexible data export options and refined UI styling, improving user interaction and data accessibility.